[DX12] Add support for Samplers in DX12. by manon-traverse · Pull Request #1331 · llvm/offload-test-suite

manon-traverse · 2026-06-24T13:59:21Z

Adds support for samplers in DX12.
This change is a lot larger than you would expect from adding samplers.
This is because samplers need to live in a separate DescriptorHeap which cascaded into a couple of issues:

Building the descriptor tables requires building tables for both sampler and CBV/SRV/UAV descriptors.
The binding of descriptor tables can no longer be implied based on the DescriptorSets defined in .test file
A layout must be generated when the root signature is generated based on the bindings
In case of root signatures defined in the shader, the layout must be extracted from that root signature by deserializing it.

Adding this feature does allow more tests to succeed on DX12.

EmilioLaiso · 2026-06-26T13:48:13Z

+enum class RootParameterType : uint32_t {
+  DescriptorTable = 0,
+  SamplerTable,
+  Constant,
+  CBV,
+  SRV,
+  UAV,
+};
+
+struct RoogtSignatureLayout {
+  RootParameterType ParameterType : 3;
+  uint32_t Count : 29;
+
+  RoogtSignatureLayout(RootParameterType ParameterType, uint32_t Count)
+      : ParameterType(ParameterType), Count(Count) {}
+  RoogtSignatureLayout() = delete;
+};
+


Could've probably been two PRs, but at least this way it doesn't only introduces new API but it also shows how it's used

MarijnS95 · 2026-06-29T14:06:43Z

-  // Depth formats require DepthStencil usage; non-depth formats forbid it.
-  if (IsDepth && !IsDS)
-    return llvm::createStringError(
-        std::errc::invalid_argument,
-        "Depth format '%s' requires DepthStencil usage.",
-        getFormatName(Desc.Fmt).data());


Out of scope / unrelated?

MarijnS95 · 2026-06-29T14:11:08Z

+        Ranges.get()[RangeIdx].OffsetInDescriptorsFromTableStart =
+            SamplerDescriptorIdx;
+
+        assert(Binding.DescriptorCount == 1 && "Manon expected this to be 1.");


Debug text?

MarijnS95 · 2026-06-29T14:11:46Z

+  UAV,
+};
+
+struct RoogtSignatureLayout {


I am groot (roogt?)

Suggested change

struct RoogtSignatureLayout {

struct RootSignatureLayout {

MarijnS95

A few more, on top of the inline notes:

Texture.h — dropping the "depth format requires DepthStencil usage" check is unrelated to samplers, and the depth-as-SRV path it enables still isn't correct (typed D32_FLOAT can't back an SRV without TYPELESS; those tests stay XFAIL). Maybe split out.
Implicit fallback now hard-errors on shader-embedded inline descriptors / root constants when no explicit RootParameters are given (previously bound each set as a table). No test hits it, but it's a behavior change.
assert(... "Manon expected this to be 1.") silently caps sampler arrays at 1 — prefer a real createStringError stating the invariant.
RoogtSignatureLayout (and the error string RootParamters) is misspelled.

Cleared on inspection: SamplerCreateDesc brace-init order matches the struct, the filter/comparison/address enum mappings are correct, and the 256-cap SamplerAllocator matches the existing allocator pattern.

MarijnS95 · 2026-06-29T14:12:52Z

@@ -0,0 +1,67 @@
+//===- Sampler.h - Offload API Texture ------------------------------------===//


Suggested change

//===- Sampler.h - Offload API Texture ------------------------------------===//

//===- Sampler.h - Offload API Sampler ------------------------------------===//

MarijnS95 · 2026-06-29T14:12:59Z

+//
+//
+//===----------------------------------------------------------------------===//


Suggested change

//

//

//===----------------------------------------------------------------------===//

MarijnS95 · 2026-06-29T14:13:10Z

+  // Sampler(Sampler &&) = delete;
+  Sampler &operator=(const Sampler &) = delete;
+  // Sampler &operator=(Sampler &&) = delete;


Suggested change

// Sampler(Sampler &&) = delete;

Sampler &operator=(const Sampler &) = delete;

// Sampler &operator=(Sampler &&) = delete;

Sampler &operator=(const Sampler &) = delete;

MarijnS95 · 2026-06-29T14:13:40Z

+                             const ComPtr<ID3D12DescriptorHeap> &SamplerHeap) {
    // Bind descriptors in descriptor tables.
-    if (!DescHeap)
+    if (!DescHeap && !SamplerHeap)


Suggested change

if (!DescHeap && !SamplerHeap)

if (!DescHeap || !SamplerHeap)

Because both are unconditionally dereferenced afterwards.

MarijnS95 · 2026-06-29T14:19:05Z

-      for (uint32_t Idx = 0u; Idx < P.Sets.size(); ++Idx) {
-        DXCB.CmdList->SetComputeRootDescriptorTable(Idx, Handle);
-        Handle.Offset(P.Sets[Idx].Resources.size(), Inc);
+      for (uint32_t I = 0, N = DXPipeline.Layout.size(); I < N; ++I) {


Claude thinks this loop can be deduplicated leaving only the graphics vs compute-specific calls on the API.

MarijnS95 · 2026-06-29T14:21:55Z

        case dx::RootParamKind::DescriptorTable:
+          // TODO(manon): Add support for descriptor tables containing samplers
          DXCB.CmdList->SetComputeRootDescriptorTable(RootParamIndex++, Handle);
          Handle.Offset(P.Sets[DescriptorTableIndex++].Resources.size(), Inc);
          break;


Samplers live in a separate heap, so a DescriptorTable mixing a sampler with CBV/SRV/UAV resources binds the wrong heap and Handle.Offset() over-advances past the descriptors actually written (no SamplerTable case here either). Hard-error until it's supported (case needs braces for the local):

Suggested change

case dx::RootParamKind::DescriptorTable:

// TODO(manon): Add support for descriptor tables containing samplers

DXCB.CmdList->SetComputeRootDescriptorTable(RootParamIndex++, Handle);

Handle.Offset(P.Sets[DescriptorTableIndex++].Resources.size(), Inc);

break;

case dx::RootParamKind::DescriptorTable: {

const DescriptorSet &Set = P.Sets[DescriptorTableIndex++];

// TODO(manon): Add support for descriptor tables containing samplers.

for (const auto &R : Set.Resources)

if (R.isSampler())

return llvm::createStringError(

"Descriptor tables containing samplers are not yet "

"supported with explicit RootParameters.");

DXCB.CmdList->SetComputeRootDescriptorTable(RootParamIndex++, Handle);

Handle.Offset(Set.Resources.size(), Inc);

break;

}

bob80905 · 2026-06-29T20:42:19Z

          break;
        case DescriptorKind::SAMPLER:
-          llvm_unreachable("Not implemented yet."); // Requires a separate heap
+          llvm_unreachable("Sampler should have been filtered out.");


I would think this is dead code because of the continue on line 1425

Yes, which is why I put the unreachable statement there. I want all cases to be handled instead of relying on the default case label.

manon-traverse force-pushed the dx12-sampler-support branch from d082a3c to 0609be6 Compare June 24, 2026 14:26

manon-traverse marked this pull request as draft June 24, 2026 15:16

manon-traverse mentioned this pull request Jun 24, 2026

Add mipmap support to createTextureWithData. #1332

Draft

manon-traverse force-pushed the dx12-sampler-support branch from 0609be6 to 273e4a5 Compare June 24, 2026 15:24

This was referenced Jun 24, 2026

[VK] Use common resource creation, readbacks, and InvocationState #1333

Draft

[Metal] Use common resource creation, readbacks, and InvocationState #1337

Draft

manon-traverse force-pushed the dx12-sampler-support branch from 273e4a5 to 909509e Compare June 25, 2026 17:29

manon-traverse marked this pull request as ready for review June 25, 2026 17:31

manon-traverse requested review from EmilioLaiso, bogner and llvm-beanz June 25, 2026 17:36

manon-traverse mentioned this pull request Jun 26, 2026

[Metal] Track resource residency using MTL::ResidencySet instead of useResource. #1340

Draft

manon-traverse force-pushed the dx12-sampler-support branch from 14ea640 to 99062f2 Compare June 26, 2026 13:37

EmilioLaiso approved these changes Jun 26, 2026

View reviewed changes

manon-traverse added 4 commits June 29, 2026 14:43

Add support for Samplers on DX12.

5ed19c6

Make clang-tidy happy.

c4b17cc

Remove debug print.

ca29bbc

Mark tests that fail on clang as XFAIL.

3c20e02

manon-traverse force-pushed the dx12-sampler-support branch from 8095bff to 3c20e02 Compare June 29, 2026 12:43

MarijnS95 reviewed Jun 29, 2026

View reviewed changes

bob80905 reviewed Jun 29, 2026

View reviewed changes

		@@ -0,0 +1,67 @@
		//===- Sampler.h - Offload API Texture ------------------------------------===//

	//===- Sampler.h - Offload API Texture ------------------------------------===//
	//===- Sampler.h - Offload API Sampler ------------------------------------===//

	//
	//
	//===----------------------------------------------------------------------===//

	if (!DescHeap && !SamplerHeap)
	if (!DescHeap \|\| !SamplerHeap)

-        case dx::RootParamKind::DescriptorTable:
-          // TODO(manon): Add support for descriptor tables containing samplers
-          DXCB.CmdList->SetComputeRootDescriptorTable(RootParamIndex++, Handle);
-          Handle.Offset(P.Sets[DescriptorTableIndex++].Resources.size(), Inc);
-          break;
+        case dx::RootParamKind::DescriptorTable: {
+          const DescriptorSet &Set = P.Sets[DescriptorTableIndex++];
+          // TODO(manon): Add support for descriptor tables containing samplers.
+          for (const auto &R : Set.Resources)
+            if (R.isSampler())
+              return llvm::createStringError(
+                  "Descriptor tables containing samplers are not yet "
+                  "supported with explicit RootParameters.");
+          DXCB.CmdList->SetComputeRootDescriptorTable(RootParamIndex++, Handle);
+          Handle.Offset(Set.Resources.size(), Inc);
+          break;
+        }

Uh oh!

Conversation

manon-traverse commented Jun 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarijnS95 Jun 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarijnS95 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

manon-traverse commented Jun 24, 2026 •

edited

Loading

MarijnS95 Jun 29, 2026 •

edited

Loading